Skip to content

Comments

feat(cli): Included new info command#1230

Open
jamesgeorge007 wants to merge 10 commits intofirebase:mainfrom
jamesgeorge007:feat/info-cmd
Open

feat(cli): Included new info command#1230
jamesgeorge007 wants to merge 10 commits intofirebase:mainfrom
jamesgeorge007:feat/info-cmd

Conversation

@jamesgeorge007
Copy link
Contributor

@jamesgeorge007 jamesgeorge007 commented May 1, 2019

Description

Included a new info command which shows debugging information about the environment which proves to be useful while submitting bug reports.

Sample Commands

firebase info

Environment Info:

  System:
    OS: Linux 4.18 Ubuntu 18.10 (Cosmic Cuttlefish)
    CPU: (4) x64 Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz
  Binaries:
    Node: 8.11.4 - /usr/bin/node
    Yarn: 1.16.0-0 - /usr/local/bin/yarn
    npm: 6.9.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 74.0.3729.108
    Firefox: 66.0.3
  npmGlobalPackages:
    firebase: 6.7.2

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label May 1, 2019
@jamesgeorge007
Copy link
Contributor Author

jamesgeorge007 commented May 1, 2019

@bkendall Thoughts 🤔

@jamesgeorge007
Copy link
Contributor Author

jamesgeorge007 commented May 1, 2019 •
edited
Loading

The CI test fails as I have used async/await which it (eslint) doesn't expect. The envinfo.run() executes in an asynchronous manner and hence, one should wait until it resolves.

@coveralls
Copy link

coveralls commented May 1, 2019

Coverage Status

Coverage remained the same at 61.557% when pulling 90a986d on jamesgeorge007:feat/info-cmd into aa39da5 on firebase:master.

1 similar comment
@coveralls
Copy link

coveralls commented May 1, 2019

Coverage Status

Coverage remained the same at 61.557% when pulling 90a986d on jamesgeorge007:feat/info-cmd into aa39da5 on firebase:master.

@coveralls
Copy link

coveralls commented May 1, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 65.054% when pulling ec0f3b9 on jamesgeorge007:feat/info-cmd into 790209c on firebase:master.

bkendall
bkendall requested changes May 1, 2019
View reviewed changes
Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. As this adds a new command, there's more internal process that has to happen as we (Firebase) are very deliberate about what we add to our public APIs, CLI commands included.

  2. This seems like it accomplishes a very similar goal to our output when the --debug flag is turned on. As "run the command with the --debug flag and provide the output" is a standard procedure for us while debugging, it may make more sense to include some of this information in that output instead. (And, as a bonus, if it's not adding a new command the friction to add this information output will be a lot lower.) What do you think?

@bkendall
Copy link
Contributor

bkendall commented May 1, 2019

@bkendall Thoughts 🤔

Did you read (2) in my previous comment?

@jamesgeorge007
Copy link
Contributor Author

jamesgeorge007 commented May 7, 2019 •
edited
Loading

@bkendall IMO having a dedicated command to show up all those local environment information makes sense rather than including it with the --debug option 🤔
Most scaffolding tools these days are shipped with a dedicated info command which turns out to be helpful when the user might want to submit a bug.

@bkendall
Copy link
Contributor

bkendall commented May 7, 2019 •
edited
Loading

@bkendall IMO having a dedicated command to show up all those local environment information makes sense rather than including it with the --debug option 🤔
Most scaffolding tools these days are shipped with a dedicated info command which turns out to be helpful when the user might want to submit a bug.

Noted, thank you. That still doesn't change (1) from above - for a new command, we have to do further review.

Do know that a lot of us are very busy with Google I/O related things, so it's unlikely anything changes this week. I can't say when an info command review would complete, but it will be measured in weeks not days.

Thanks for your contribution, but it does take time to get all the pieces together when adding a new command... thanks for your patience.

@jamesgeorge007
Copy link
Contributor Author

jamesgeorge007 commented May 7, 2019

@bkendall I can understand 😃

@jamesgeorge007 jamesgeorge007 changed the title feat(cli): Added new info command feat(cli): Included new info command Jun 29, 2019
@jamesgeorge007
Copy link
Contributor Author

jamesgeorge007 commented Jul 14, 2019

@bkendall Guess we're good to go after #105 gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkendall bkendall Awaiting requested review from bkendall

Requested changes must be addressed to merge this pull request.

Assignees

@jamesgeorge007 jamesgeorge007

Labels

cla: yes Manual indication that this has passed CLA.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jamesgeorge007 @coveralls @bkendall @googlebot